Remove kube-rbac-proxy #8302

pebrc · 2024-12-06T15:27:37Z

This removes the kube-auth-proxy from the ECK Helm charts and the ECK documentation. Instead if follows the recommendation from controller-runtime to use the built-in FilterProvider filters.WithAuthenticationAndAuthorization.

This pulls in a bunch of k8s API server dependencies increasing the binary size by about 12MB IIRC.

I have also tried to address some issues with our current Helm templating of the metrics server:

allow enabling the secure mode with TLS+auth while not forcing users to have Promotheus installed e.g. when using Elastic Agent (!) (service monitor is still generated by default for bwc)
mixing configuration properties for the service monitor with the configuration properties of the metrics server (I moved them to serviceMonitor.* while implementing a form of bwc layer in the template

Things I did not address:

the mutual exclusion between secure metrics and the pod monitor (see my comment below)

Tests I did (all with Prometheus operator):

Helm installation with auto-gen self-signed certs insecureSkipVerify: true
Manual installation with auto-gen self-signed certs insecureSkipVerify: true
Manual installation with cert-manager certs + modified Prometheus deployment and insecureSkipVerify: false

Review
If you can test a few scenarios while reviewing this it would much appreciated to flush out any issues I have missed.

deploy/eck-operator/values.yaml

docs/operating-eck/configure-operator-metrics.asciidoc

naemono · 2024-12-09T19:32:17Z

I'll look at this today @pebrc

cmd/manager/main.go

naemono · 2024-12-10T00:03:24Z

I'll look at this today @pebrc

When testing with the following values

config:
  metrics:
    port: 8443
    secureMode:
      enabled: true
image:
  repository: docker.elastic.co/eck-ci/eck-operator-pr
  tag: 8302-b2670646
installCRDs: true
managedNamespaces: []
webhook:
  enabled: true

I see this working without issues:

I'll have to test with more scenarios tomorrow and update.

deploy/eck-operator/templates/service-monitor.yaml

naemono · 2024-12-10T17:43:12Z

From my testing the option for providing your own certificate/key/ca is broken, as it was configured in the rbac-proxy in the previous implementation. I'm working through what the fix looks like for this and will update.

Signed-off-by: Michael Montgomery <mmontg1@gmail.com>

naemono · 2024-12-10T19:47:52Z

From my testing the option for providing your own certificate/key/ca is broken, as it was configured in the rbac-proxy in the previous implementation. I'm working through what the fix looks like for this and will update.

I've pushed a commit that fixes the issue when using custom certificates:

Using the following values:

config:
  metrics:
    port: 8443
    secureMode:
      enabled: true
      tls:
        certificateSecret: eck-metrics-tls-certificate
image:
  repository: docker.elastic.co/eck-ci/eck-operator-pr
  tag: 8302-b2670646
installCRDs: true
managedNamespaces: []
podMonitor:
  enabled: false
serviceMonitor:
  caSecret: eck-metrics-tls-ca
  enabled: true
  insecureSkipVerify: false
webhook:
  enabled: true

pebrc · 2024-12-10T20:18:32Z

From my testing the option for providing your own certificate/key/ca is broken, as it was configured in the rbac-proxy in the previous implementation. I'm working through what the fix looks like for this and will update.

I've pushed a commit that fixes the issue when using custom certificates:

Using the following values:
config:
  metrics:
    port: 8443
    secureMode:
      enabled: true
      tls:
        certificateSecret: eck-metrics-tls-certificate
image:
  repository: docker.elastic.co/eck-ci/eck-operator-pr
  tag: 8302-b2670646
installCRDs: true
managedNamespaces: []
podMonitor:
  enabled: false
serviceMonitor:
  caSecret: eck-metrics-tls-ca
  enabled: true
  insecureSkipVerify: false
webhook:
  enabled: true

Good catch. I tested the custom certs only with the manual mode. I think there is a simpler fix though by just using the default path from controller-runtime in the volume mount.

naemono · 2024-12-10T20:19:23Z

Good catch. I tested the custom certs only with the manual mode. I think there is a simpler fix though by just using the default path from controller-runtime in the volume mount.

I'll take a look at this and update... thanks

This reverts commit 42e0d56.

naemono · 2024-12-11T02:16:44Z

Good catch. I tested the custom certs only with the manual mode. I think there is a simpler fix though by just using the default path from controller-runtime in the volume mount.

I'll take a look at this and update... thanks

i sat down to finish this and saw you handled it. i will verify in the am. thanks

deploy/eck-operator/templates/service-monitor.yaml

cmd/manager/main.go

docs/operating-eck/configure-operator-metrics.asciidoc

barkbay

Nice work! I really like the additional fixes/improvements you included in this PR 👍

I ran a couple of tests, only with Helm, using both the ECK and the Prometheus operator charts, with a custom CA, enforcing (or not) TLS validation, and it worked as expected.

docs/operating-eck/configure-operator-metrics.asciidoc

thbkrkr

Looks good 👍

cmd/manager/main.go

thbkrkr · 2024-12-11T12:31:05Z

docs/operating-eck/configure-operator-metrics.asciidoc

Not from your PR, I noticed the metadata.labels of the Service and the spec.selector.matchLabels of the ServiceMonitor in configure-operator-metrics.asciidoc are not aligned with what is rendered using the Helm chart.

This removes the kube-auth-proxy from the ECK Helm charts and the ECK documentation. Instead if follows the recommendation from controller-runtime to use the built-in FilterProvider filters.WithAuthenticationAndAuthorization. This pulls in a bunch of k8s API server dependencies increasing the binary size by about 12MB IIRC. I have also tried to address some issues with our current Helm templating of the metrics server: - allow enabling the secure mode with TLS+auth while not forcing users to have Promotheus installed e.g. when using Elastic Agent (!) (service monitor is still generated by default for bwc) - mixing configuration properties for the service monitor with the configuration properties of the metrics server (I moved them to serviceMonitor.* while implementing a form of bwc layer in the template --------- Co-authored-by: Michael Montgomery <mmontg1@gmail.com> Co-authored-by: Michael Morello <michael.morello@elastic.co> (cherry picked from commit 032bff5) # Conflicts: # go.sum

pebrc · 2024-12-11T13:13:00Z

💚 All backports created successfully

Status	Branch	Result
✅	2.16

Questions ?

Please refer to the Backport tool documentation

This removes the kube-auth-proxy from the ECK Helm charts and the ECK documentation. Instead if follows the recommendation from controller-runtime to use the built-in FilterProvider filters.WithAuthenticationAndAuthorization. This pulls in a bunch of k8s API server dependencies increasing the binary size by about 12MB IIRC. I have also tried to address some issues with our current Helm templating of the metrics server: - allow enabling the secure mode with TLS+auth while not forcing users to have Promotheus installed e.g. when using Elastic Agent (!) (service monitor is still generated by default for bwc) - mixing configuration properties for the service monitor with the configuration properties of the metrics server (I moved them to serviceMonitor.* while implementing a form of bwc layer in the template --------- Co-authored-by: Michael Montgomery <mmontg1@gmail.com> Co-authored-by: Michael Morello <michael.morello@elastic.co> (cherry picked from commit 032bff5)

Remove kube-rbac-proxy

1a925a4

botelastic bot added the triage label Dec 6, 2024

pebrc added 2 commits December 6, 2024 16:29

Adjust operator config

a41e8b7

missing file

8e94412

pebrc added the >enhancement Enhancement of existing functionality label Dec 6, 2024

botelastic bot removed the triage label Dec 6, 2024

pebrc added 6 commits December 6, 2024 17:13

Merge remote-tracking branch 'upstream' into metrics-auth

b1421a9

enable TLS

5dd0562

re-generate

adcaae1

Helm chart restructuring

b4160c2

Doc changes

78dc787

Remove references to "proxy" from docs and templates

b267064

pebrc marked this pull request as ready for review December 9, 2024 15:50

pebrc commented Dec 9, 2024

View reviewed changes

deploy/eck-operator/values.yaml Show resolved Hide resolved

pebrc commented Dec 9, 2024

View reviewed changes

docs/operating-eck/configure-operator-metrics.asciidoc Show resolved Hide resolved

pebrc requested review from naemono, thbkrkr, barkbay, rhr323 and kvalliyurnatt December 9, 2024 19:31

naemono reviewed Dec 10, 2024

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

naemono reviewed Dec 10, 2024

View reviewed changes

deploy/eck-operator/templates/service-monitor.yaml Show resolved Hide resolved

Ensure metrics-cert-dir is set when certificateSecret is set.

42e0d56

Signed-off-by: Michael Montgomery <mmontg1@gmail.com>

Revert "Ensure metrics-cert-dir is set when certificateSecret is set."

7c6a7c6

This reverts commit 42e0d56.

Use controller-runtime default path in volume mount

b42cb61

barkbay reviewed Dec 11, 2024

View reviewed changes

deploy/eck-operator/templates/service-monitor.yaml Show resolved Hide resolved

barkbay reviewed Dec 11, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

barkbay reviewed Dec 11, 2024

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

barkbay reviewed Dec 11, 2024

View reviewed changes

docs/operating-eck/configure-operator-metrics.asciidoc Outdated Show resolved Hide resolved

barkbay approved these changes Dec 11, 2024

View reviewed changes

docs/operating-eck/configure-operator-metrics.asciidoc Outdated Show resolved Hide resolved

pebrc added 3 commits December 11, 2024 09:51

Document operator flags

e5437e8

Review fixes to docs

60ff77a

values.yaml doc tweaks

a108ee3

pebrc added the v2.16.0 label Dec 11, 2024

thbkrkr approved these changes Dec 11, 2024

View reviewed changes

pebrc merged commit 032bff5 into elastic:main Dec 11, 2024
5 checks passed

pebrc mentioned this pull request Dec 11, 2024

[2.16] Remove kube-rbac-proxy (#8302) #8313

Merged

naemono mentioned this pull request Dec 11, 2024

Adjust 2.16 release notes from recent changes. #8316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove kube-rbac-proxy #8302

Remove kube-rbac-proxy #8302

pebrc commented Dec 6, 2024 •

edited

Loading

naemono commented Dec 9, 2024

naemono commented Dec 10, 2024

naemono commented Dec 10, 2024

naemono commented Dec 10, 2024

pebrc commented Dec 10, 2024

naemono commented Dec 10, 2024

naemono commented Dec 11, 2024

barkbay left a comment

thbkrkr left a comment

thbkrkr Dec 11, 2024

pebrc commented Dec 11, 2024

Remove kube-rbac-proxy #8302

Remove kube-rbac-proxy #8302

Conversation

pebrc commented Dec 6, 2024 • edited Loading

naemono commented Dec 9, 2024

naemono commented Dec 10, 2024

naemono commented Dec 10, 2024

naemono commented Dec 10, 2024

pebrc commented Dec 10, 2024

naemono commented Dec 10, 2024

naemono commented Dec 11, 2024

barkbay left a comment

Choose a reason for hiding this comment

thbkrkr left a comment

Choose a reason for hiding this comment

thbkrkr Dec 11, 2024

Choose a reason for hiding this comment

pebrc commented Dec 11, 2024

💚 All backports created successfully

Questions ?

pebrc commented Dec 6, 2024 •

edited

Loading